Printing String objects as strings instead of objects #1450
Conversation
There was a problem hiding this comment.
It's absurd to ever use boxed primitives in JavaScript; but that said, enzyme should certainly handle it well even if people do bad things.
The console has superpowers; if we want to detect boxed strings, we have to have special-cased code for it.
However, another alternative is to use the object-inspect library for the "object" case, which already handles all boxed primitives, and might allow the Debug code to be greatly simplified.
packages/enzyme/src/Debug.js
Outdated
| @@ -32,7 +32,11 @@ function propString(prop) { | |||
| case 'boolean': | |||
| return `{${prop}}`; | |||
| case 'object': | |||
| return '{{...}}'; | |||
| if (prop instanceof String) { | |||
There was a problem hiding this comment.
instanceof is never reliable, and should not be used.
If we want to detect boxed strings, we need to use the is-string library.
packages/enzyme/src/Debug.js
Outdated
| @@ -32,7 +32,11 @@ function propString(prop) { | |||
| case 'boolean': | |||
| return `{${prop}}`; | |||
| case 'object': | |||
| return '{{...}}'; | |||
| if (prop instanceof String) { | |||
| return `"${prop.toString()}"`; | |||
There was a problem hiding this comment.
String(x) is always strictly better than x.toString(), and inside a template literal, both are unnecessary for a non-Symbol - this can just be `"${prop}"`
|
@ljharb thanks for the feedback - just pushed some changes. |
packages/enzyme/src/Debug.js
Outdated
| return '{{...}}'; | ||
| if (isString(prop)) { | ||
| return `"${prop}"`; | ||
| } else { |
There was a problem hiding this comment.
The else here isn’t necessary; the following return can be de-indented
ljharb
force-pushed
the
string-object-debug
branch
from
7e2fd4f
to
8533b5e
Compare
This pull request updates the way that nodes are printed when being debugged.
Use case in question
Previous behavior
Debugging the above node previously logged the following:
Proposed behavior
I propose it would make more sense to print props that are String objects as strings:
Notes
Providing String objects for element props like className is something that is supported by the DOM, and thus supported by React.